-
-
Notifications
You must be signed in to change notification settings - Fork 858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IMP] Add tests for base_location name_search #585
Conversation
But the name_search is not modified in this PR... |
5c85c08
to
95d636d
Compare
Now it could be ok! |
And change the module version number... |
The problem with this PR is that you are breaking inheritance. The problem with name_search is that is very tricky. Check how we were force to do it in res.partner for l10n_es_partner: |
Now I'm really confused, what do you mean it breaks inheritance? It hase the same interface as in models.Model, and it's actually implemented exactly in the same way as in models.Model, pretty much line by line (it just skips access rights tests that in this case are not necessary)! What am I missing? For reference, current models.Model implementation is: @api.model
def _name_search(self, name='', args=None, operator='ilike', limit=100, name_get_uid=None):
# private implementation of name_search, allows passing a dedicated user
# for the name_get part to solve some access rights issues
args = list(args or [])
# optimize out the default criterion of ``ilike ''`` that matches everything
if not self._rec_name:
_logger.warning("Cannot execute name_search, no _rec_name defined on %s", self._name)
elif not (name == '' and operator == 'ilike'):
args += [(self._rec_name, operator, name)]
access_rights_uid = name_get_uid or self._uid
ids = self._search(args, limit=limit, access_rights_uid=access_rights_uid)
recs = self.browse(ids)
return recs.sudo(access_rights_uid).name_get() |
That's the base model. When you don't call super in your module, and there's other module overwriting the same method, both conflict and you don't know which of the 2 is going to be executed. |
Your argument is actually true for each and every method in a class, and for the little I understand it's a problem intrinsic to the the multi-inheritance scheme odoo is using which bypasses normal pythonic inheritance (in a sane world, inheritance should be predictable, deterministic and hopefully in cortrol of the programmer). The solution you pointed in l10n-spain is not really solving the problem you described: it's a workaround that actually leaves you with partially undefined behaviour anyway, since you don't know what results you get first if someone else is exending ResPartner again, in the end virtually any subclass is adding search results in a potentially random order and nobody knows what's really coming out of that name_search call. Besides, in that example super is called only if the new search is not producing enough results, not even everytime, so the original behaviour is not always maintained. Moreover, in the example you pointed out, an existing model, ResPartner, is being extended, hence you do want to extend rather than replace the behaviour of the model being extended. BetterZip is not extending an existing model, it's introducing a new one, so this class is actually the one defining the behaviour of this model, not the one extending it. Your argument could be applied to any class exending BetterZip though... |
You are totally right, as the model is first defined here! Sorry for the noise. |
No worries, and thanks for the thorough review! Your comments forced me to go back to check and learn lots of stuff i overlooked, which in the end is really really helpful! :) |
[FIX] disable position editing on contact - This is just to see all positions - If we want to add position go to one company and add contact linked to this partner [ADD] setup.py [ci skip] [MIG][11.0] Migration of partner_affiliate to v11 (#579) * [MIG] Migration of partner_affiliate to v11 * Fixing use of @Class and do not inheriting address from parent company * Improving legibility and details of fields * Fixing travis errors [ADD] setup.py [IMP] base_location: Include onchange for state Incredibly not included in Odoo core. [IMP] base_location: name_search improvement (#585) Fixing travis errors
Continuing here the discussion/development of MR
#582
which was closed by mistake.